Skip to content

Test case & fix for getting items from empty dictionary. SIGBUS & SIGSEGV on Mac #368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
narekgharibyan merged 9 commits into from
Oct 8, 2025
Merged

Test case & fix for getting items from empty dictionary. SIGBUS & SIGSEGV on Mac #368

narekgharibyan merged 9 commits into from
Oct 8, 2025

Conversation

@narekgharibyan
Copy link
Member

@narekgharibyan narekgharibyan commented Oct 7, 2025

Opening this PR as continuation to narekgharibyan#2.

Unfortunately even after #359 cargo test command fails on this branch.

➜  rust git:(get_all_items_empty) cargo test
.
.
.
warning: `keyvi` (test "tests") generated 1 warning (run `cargo fix --test "tests"` to apply 1 suggestion)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.08s
     Running unittests src/lib.rs (target/debug/deps/keyvi-fcb4ba69b4eddeda)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/tests.rs (target/debug/deps/tests-76ca346cb5ae4236)

running 23 tests
dictionary file not found
test tests::dictionary_error ... ok
test tests::get_all_items ... ok
test tests::fuzzy_completions_with_score ... ok
test tests::dictionary_size ... ok
error: test failed, to rerun pass `--test tests`

Caused by:
  process didn't exit successfully: `/Users/narek/projects/keyvi-1/rust/target/debug/deps/tests-76ca346cb5ae4236` (signal: 10, SIGBUS: access to undefined memory)

Another run

➜  rust git:(get_all_items_empty) cargo test
.
.
.
warning: `keyvi` (test "tests") generated 1 warning (run `cargo fix --test "tests"` to apply 1 suggestion)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.03s
     Running unittests src/lib.rs (target/debug/deps/keyvi-fcb4ba69b4eddeda)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/tests.rs (target/debug/deps/tests-76ca346cb5ae4236)

running 23 tests
dictionary file not found
test tests::dictionary_error ... ok
error: test failed, to rerun pass `--test tests`

Caused by:
  process didn't exit successfully: `/Users/narek/projects/keyvi-1/rust/target/debug/deps/tests-76ca346cb5ae4236` (signal: 11, SIGSEGV: invalid memory reference)

I tried and the situation is following:

So unfortunately it seems we still have an issue at the moment :(

@narekgharibyan narekgharibyan mentioned this pull request Oct 7, 2025
Closed
@hendrikmuhs
Copy link
Contributor

hendrikmuhs commented Oct 7, 2025

rust/test_data/empty-key-dict.kv looks like an old dict, meaning it has been created < 0.7.2.

Nevertheless, I thought #359 works backwards compatible for the case: old dict + new library.

@hendrikmuhs
Copy link
Contributor

hendrikmuhs commented Oct 7, 2025

Problem:

keyvi stats test_data/empty-key-dict.kv
{
    "General": {
        "number_of_keys": 0,
        "number_of_states": 1,
        "start_state": 1,
        "value_store_type": 1,
        "version": 2
    },
    "Persistence": {
        "size": 262,
        "version": 2
    },
    "Value Store": {
        "size": 0,
        "unique_values": 0,
        "values": 0
    }
}

"number_of_states": 1: I forgot that old dicts still have a state, that's wrong, an empty dict should have no states(>= 0.7.2 fixes this, too). But that's how it is. It would be nice to fix this.

number_of_states is used here:

    return dictionary_properties_->GetNumberOfStates() != 0 ? dictionary_properties_->GetStartState() : 0;

Using number_of_keys instead, would fix this issue.

@narekgharibyan
Copy link
Member Author

narekgharibyan commented Oct 7, 2025

@hendrikmuhs is 4ec9a76 what you had in mind ?

@hendrikmuhs
Copy link
Contributor

hendrikmuhs commented Oct 8, 2025

@hendrikmuhs is 4ec9a76 what you had in mind ?

👍 yes, that will fix this BWC case.

FWIW: The python tests work end to end, so compile an empty dict. As rust misses the compiler parts, the added binary file has been created with an older compiler, that's why it covers the BWC case.

I will think about improving BWC testing, but for now lets merge that fix.

hendrikmuhs
hendrikmuhs reviewed Oct 8, 2025
View reviewed changes
@narekgharibyan @hendrikmuhs
Update rust/tests/tests.rs
a68c8a8 
Co-authored-by: Hendrik Muhs <[email protected]>
@narekgharibyan
Copy link
Member Author

narekgharibyan commented Oct 8, 2025

Cool, yeah will merge and attempt a release once tests are green!

There is some sort of weird satisfaction that the test case I created 3 years ago identified that edge case :D

@narekgharibyan narekgharibyan changed the title Test case for getting items from empty dictionary. SIGBUS & SIGSEGV on Mac Test case & fix for getting items from empty dictionary. SIGBUS & SIGSEGV on Mac Oct 8, 2025
@narekgharibyan narekgharibyan merged commit 3acaf1e into KeyviDev:master Oct 8, 2025
17 checks passed
@narekgharibyan narekgharibyan deleted the get_all_items_empty branch October 8, 2025 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hendrikmuhs hendrikmuhs hendrikmuhs approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@narekgharibyan @hendrikmuhs